[DebugInfo] Don't apply is_stmt on MBB branches that preserve lines #108251
Conversation
SLTozer
requested review from
jryans,
jmorse,
adrian-prantl,
OCHyams,
dwblaikie and
pogo59
|
@llvm/pr-subscribers-debuginfo Author: Stephen Tozer (SLTozer) ChangesThis patch follows on from the changes made in 5fef40c, by adding an additional heuristic that prevents us from applying the start-of-MBB is_stmt flag when we can see that, for all direct branches to the MBB, the last line stepped on before the branch is the same as the first line of the MBB. This is mainly to prevent certain pathological cases, such as macros that expand to multiple basic blocks that all have the same source location, from giving us repeated steps on the same line. This approach is not comprehensive, since it relies on analyzeBranch to read edges. Full diff: https://github.com/llvm/llvm-project/pull/108251.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 148b620c2b62b7..b0139e19a8498c 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -2062,7 +2062,9 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
bool PrevInstInDiffBB = PrevInstBB && PrevInstBB != MI->getParent();
- if (DL == PrevInstLoc && !PrevInstInDiffBB) {
+ bool ForceIsStmt =
+ PrevInstInDiffBB && MBBsStartingWithIsStmt.contains(MI->getParent());
+ if (DL == PrevInstLoc && !ForceIsStmt) {
// If we have an ongoing unspecified location, nothing to do here.
if (!DL)
return;
@@ -2120,7 +2122,7 @@ void DwarfDebug::beginInstruction(const MachineInstr *MI) {
// mark is_stmt for the first non-0 line in each BB, in case a predecessor BB
// ends with a different line.
unsigned OldLine = PrevInstLoc ? PrevInstLoc.getLine() : LastAsmLine;
- if (DL.getLine() && (DL.getLine() != OldLine || PrevInstInDiffBB))
+ if (DL.getLine() && (DL.getLine() != OldLine || ForceIsStmt))
Flags |= DWARF2_FLAG_IS_STMT;
const MDNode *Scope = DL.getScope();
@@ -2228,6 +2230,119 @@ void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
// Record beginning of function.
PrologEndLoc = emitInitialLocDirective(
*MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
+
+ // Try to determine what line we would be stepped on before entering each MBB.
+ // This happens in the following ways:
+ // - If this block has a single predecessor, we determine the last line in
+ // the pred block and we have stepped from that.
+ // - If this block has multiple predecessors, we determine the last line in
+ // each of them; if they all agree then we have stepped from that line,
+ // otherwise we do not know what line we have stepped from.
+ // The last line in each MBB is determined as follows:
+ // - If the block contains at least one DebugLoc, we have stepped from the
+ // last one.
+ // - Otherwise, the last line is line 0.
+ // There is one extra rule however: if a predecessor branches to the current
+ // basic block, we only count DebugLocs on or before that branch, so if we're
+ // looking at the MBB %bb.0, which ends with:
+ // JCC_1 %bb.1, 0, !debug-location !1
+ // JMP_1 %bb.2, !debug-location !2
+ // We would consider !1 to be the last loc before %bb.1, and !2 before %bb.2.
+ // We also don't need to make this calculation for any block that doesn't have
+ // a non-0 line number on its first instruction, as we will always emit line 0
+ // without is_stmt for that block regardless.
+ MBBsStartingWithIsStmt.clear();
+
+ // First, collect the last stepped line for each MBB.
+ SmallDenseMap<std::pair<const MachineBasicBlock *, const MachineBasicBlock *>,
+ unsigned>
+ MBBExitLines;
+ const auto *TII = MF->getSubtarget().getInstrInfo();
+
+ // We only need to examine MBBs that could have is_stmt set by this logic.
+ // We use const_cast even though we won't actually modify MF, because some
+ // methods we need take a non-const MBB.
+ SetVector<MachineBasicBlock *> PredMBBsToExamine;
+ SmallVector<MachineBasicBlock *> PotentialIsStmtMBBs;
+ for (auto &MBB : *const_cast<MachineFunction *>(MF)) {
+ if (MBB.pred_empty() || !MBB.begin()->getDebugLoc())
+ continue;
+ unsigned StartLine = MBB.begin()->getDebugLoc()->getLine();
+ if (StartLine == 0)
+ continue;
+ PotentialIsStmtMBBs.push_back(&MBB);
+ for (auto Pred : MBB.predecessors())
+ PredMBBsToExamine.insert(&*Pred);
+ }
+
+ // For each predecessor MBB, we examine the last DebugLoc seen before each
+ // branch or logical fallthrough. We're generous with applying is_stmt; if
+ // there's an edge that TargetInstrInfo::analyzeBranch can't understand, we
+ // simply assume that is_stmt ought to be applied to the successors, since
+ // this rule is mainly intended to avoid excessive stepping on lines that
+ // expand to many lines of code.
+ for (auto *MBB : PredMBBsToExamine) {
+ assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
+ MachineBasicBlock *TBB = nullptr, *FBB = nullptr;
+ SmallVector<MachineOperand, 4> Cond;
+ if (TII->analyzeBranch(*MBB, TBB, FBB, Cond)) {
+ // We can't determine what DLs this branch's successors use, so skip it.
+ continue;
+ }
+ assert(TBB && "Bad result from analyzeBranch?");
+ auto MI = MBB->rbegin();
+ // For a conditional branch followed by unconditional branch where the
+ // unconditional branch has a DebugLoc, that loc is the outgoing loc to the
+ // false destination only; otherwise, both destinations share an outgoing
+ // loc.
+ if (!Cond.empty() && FBB != nullptr && MBB->back().getDebugLoc()) {
+ assert(MI->isBranch() && "Bad result from analyzeBranch?");
+ MBBExitLines.insert({{MBB, FBB}, MBB->back().getDebugLoc()->getLine()});
+ FBB = nullptr;
+ ++MI;
+ } else if (!Cond.empty() && !FBB) {
+ // For a conditional branch that falls through to the next block, the
+ // fallthrough block is the false branch.
+ FBB = MBB->getLogicalFallThrough();
+ assert(FBB &&
+ "Block ending with just a conditional branch should fallthrough.");
+ }
+
+ // If we don't find an outgoing loc, this block will start with a line 0.
+ unsigned LastLine = 0;
+ while (MI != MBB->rend()) {
+ if (auto DL = MI->getDebugLoc()) {
+ LastLine = DL->getLine();
+ break;
+ }
+ ++MI;
+ }
+ MBBExitLines.insert({{MBB, TBB}, LastLine});
+ if (FBB)
+ MBBExitLines.insert({{MBB, FBB}, LastLine});
+ }
+
+ // Now use the outgoing values to determine the incoming values for each
+ // block.
+ MBBsStartingWithIsStmt.insert(&*MF->begin());
+ for (auto *MBB : PotentialIsStmtMBBs) {
+ assert(!MBB->empty() && "Shouldn't be processing empty blocks here.");
+ if (!MBB->begin()->getDebugLoc())
+ continue;
+ unsigned StartLine = MBB->begin()->getDebugLoc()->getLine();
+ if (StartLine == 0)
+ continue;
+ for (auto Pred : MBB->predecessors()) {
+ auto LineIt = MBBExitLines.find({&*Pred, MBB});
+ // If there is no entry, it means there's a branch here that we couldn't
+ // track, so we can't be sure about what line we're arriving from;
+ // therefore assume that we should use is_stmt.
+ if (LineIt == MBBExitLines.end() || LineIt->second != StartLine) {
+ MBBsStartingWithIsStmt.insert(MBB);
+ break;
+ }
+ }
+ }
}
unsigned
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index 19f5b677bb8d06..80a6c7bd34d914 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -382,6 +382,8 @@ class DwarfDebug : public DebugHandlerBase {
SmallPtrSet<const MDNode *, 2>>;
DenseMap<const DILocalScope *, MDNodeSet> LocalDeclsPerLS;
+ SmallDenseSet<const MachineBasicBlock *> MBBsStartingWithIsStmt;
+
/// If nonnull, stores the current machine function we're processing.
const MachineFunction *CurFn = nullptr;
diff --git a/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll b/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
index f4e98512675d9d..ca9305d476d01f 100644
--- a/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
+++ b/llvm/test/DebugInfo/X86/is_stmt-at-block-start.ll
@@ -1,7 +1,8 @@
;; Checks that when an instruction at the start of a BasicBlock has the same
;; DebugLoc as the instruction at the end of the previous BasicBlock, we add
;; is_stmt to the new line, to ensure that we still step on it if we arrive from
-;; a BasicBlock other than the immediately preceding one.
+;; a BasicBlock other than the immediately preceding one, unless all known
+;; predecessor BasicBlocks end with the same line.
; RUN: %llc_dwarf -mtriple=x86_64-unknown-linux -O0 -filetype=obj < %s | llvm-dwarfdump --debug-line - | FileCheck %s
@@ -24,6 +25,25 @@ if.end8: ; preds = %if.then2
ret void
}
+; CHECK: {{0x[0-9a-f]+}} 113 5 {{.+}} is_stmt
+; CHECK-NOT: {{0x[0-9a-f]+}} 113 5
+
+define void @_Z1gi(i1 %cond) !dbg !31 {
+entry:
+ br i1 %cond, label %if.then2, label %if.else4, !dbg !34
+
+if.then2: ; preds = %entry
+ br label %if.end8, !dbg !34
+
+if.else4: ; preds = %entry
+ %0 = load i32, ptr null, align 4, !dbg !34
+ %call5 = call i1 null(i32 %0)
+ ret void
+
+if.end8: ; preds = %if.then2
+ ret void
+}
+
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!20}
@@ -35,3 +55,8 @@ if.end8: ; preds = %if.then2
!23 = !{null}
!24 = !DILocation(line: 13, column: 5, scope: !25)
!25 = distinct !DILexicalBlock(scope: !21, file: !1, line: 11, column: 27)
+!31 = distinct !DISubprogram(name: "g", linkageName: "_Z1gi", scope: !1, file: !1, line: 107, type: !32, scopeLine: 7, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!32 = distinct !DISubroutineType(types: !33)
+!33 = !{null}
+!34 = !DILocation(line: 113, column: 5, scope: !35)
+!35 = distinct !DILexicalBlock(scope: !31, file: !1, line: 111, column: 27)
|
|
Stephen wrote this patch at my request; we have code that will now (after the previous patch to set is_stmt on each bb) stop multiple times on the same line, because of a macro that expands to multiple bbs. Because I asked for it, but it looks more complex than I was hoping, I think other people should review it. Also, can we get some compile-time performance data? It must degrade some. |
Is there an argument that it might be reasonable to leave this decision to the debugger? |
Possibly, but there might be a justification to do this regardless for the purposes of reducing file size. This patch has a performance cost of 0.17% for |
|
Updated the patch and added a fix - it has a negligible performance cost for ReleaseLTO-g builds on the compile time tracker, and combined with the previous patch there is a 0.05% performance cost when combined with the previous patch. It has a larger cost at O0, but it is also necessary for significantly reducing .debug_line inflation, and the original bug was reported at O0 so I'd personally say that the patches collectively are worthwhile. This depends on review somewhat, though. |
|
Small ping for @dwblaikie - I'm preparing to revert the previous commit (some tests that were updated since it landed have to be updated), with the intent to reland the changes as part of this PR. Since I've not been able to reproduce the file size increases that you noted, are you able to test this patch to see if it reduces the impact of the previous patch? |
…on in BB (#105524)" Reverted due to large .debug_line size regressions for some configurations; work currently in place to improve the output of this behaviour in PR #108251. This patch also modifies two tests that were created or modified after the original commit landed and are affected by the revert: llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll llvm/test/DebugInfo/X86/empty-line-info.ll This reverts commit 5fef40c.
Hrm :/ what did you try testing? I'll see if I can find someone to investigate this on our side to get more data... |
Simple stage 2 build of clang, tried Debug and RelWithDebInfo with CMake defaults, with the commit applied and reverted. I didn't do split dwarf, but I don't think that should affect the line table size? If you have any more info on the config I might be able to try it out myself. |
…on in BB (llvm#105524)" Reverted due to large .debug_line size regressions for some configurations; work currently in place to improve the output of this behaviour in PR llvm#108251. This patch also modifies two tests that were created or modified after the original commit landed and are affected by the revert: llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll llvm/test/DebugInfo/X86/empty-line-info.ll This reverts commit 5fef40c.
|
I tested this PR on the case reported by dwblaikie, where we had been seeing a large regression in the size of .debug_lines. With this PR we are no longer seeing the size regression. |
…on in BB (llvm#105524)" Reverted due to large .debug_line size regressions for some configurations; work currently in place to improve the output of this behaviour in PR llvm#108251. This patch also modifies two tests that were created or modified after the original commit landed and are affected by the revert: llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll llvm/test/DebugInfo/X86/empty-line-info.ll This reverts commit 5fef40c.
SLTozer
force-pushed
the
is_stmt-pred-blocks
branch
from
ae0dfe4 to
59b4657
Compare
|
Ping, did a rebase and made some minor fixes. |
There was a problem hiding this comment.
This is looking good, although I reckon all the new code should be factored into a new named method so that readers of beginFunctionImpl get a better overview of what's going on.
Is there a reason for examining successors of interesting blocks found, instead of examining the predecessor of all blocks? Somehow the latter feels more natural to me; I don't think there's a technical reason for doing it like that though.
I wonder whether there's a topological way of avoiding some of the work, i.e. if we explore in RPO then we might have already found a block that needs is_stmt and can skip analysing it again for other predecessors? That might be overcomplicating it if this doesn't fire very often though.
It pains me to say this but... we must still explicitly skip meta-instructions post-isel to avoid variable location info leaking into the linetables.
| // For each MBB we try to determine whether and what source line is *always* | ||
| // the last stepped-on line before entering MBB. Such a line exists if every |
There was a problem hiding this comment.
This is nitpicking, but I feel the order of this sentence should be flipped to make it clear at the start what's going on -- i.e. "Find blocks where the line stepped on before entering it isn't always the same". Otherwise it's quite oblique to work out what the subject is, until the end.
| // We only need to examine MBBs that could have is_stmt set by this logic. | ||
| // We use const_cast even though we won't actually modify MF, because some | ||
| // methods we need take a non-const MBB. | ||
| SetVector<MachineBasicBlock *> PredMBBsToExamine; |
| ;; Checks that when an instruction at the start of a BasicBlock has the same | ||
| ;; DebugLoc as the instruction at the end of the previous BasicBlock, we add | ||
| ;; is_stmt to the new line, to ensure that we still step on it if we arrive from | ||
| ;; a BasicBlock other than the immediately preceding one, unless all known | ||
| ;; predecessor BasicBlocks end with the same line. | ||
|
|
There was a problem hiding this comment.
Convention is to put the RUN lines above this IIRC? Either way I endorse every test having an explanation of what it's testing.
The reason we work from the predecessors is that we need to run We could use RPO, but it would probably be just as or more efficient to just add a slightly-faster early exit at the top of the loop - it's trivial to check for each predecessor whether any of its successors are in the |
|
So before landing this, I'll note the state of its performance characteristics in case anyone comes looking after it's landed - for optimized debug builds, the performance cost of this patch is negligible. For O0 debug builds, it has a performance cost of around 0.15%; I don't see too many ways of optimizing it further without compromising correctness in some cases though, since AFAICT just the cost of iterating over the instructions is enough to noticeably slow down O0 builds. Since this is necessary to fix an O0 debug info bug (#104695) I think the cost is probably worth it. |
This patch follows on from the changes made in 5fef40c, by adding an additional heuristic that prevents us from applying the start-of-MBB is_stmt flag when we can see that, for all direct branches to the MBB, the last line stepped on before the branch is the same as the first line of the MBB. This is mainly to prevent certain pathological cases, such as macros that expand to multiple basic blocks that all have the same source location, from giving us repeated steps on the same line. This approach is not comprehensive, since it relies on analyzeBranch to read edges.
SLTozer
force-pushed
the
is_stmt-pred-blocks
branch
from
84359bb to
843dae8
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/10372 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/179/builds/10108 Here is the relevant piece of the build log for the reference |
Flow blocks are generated code that don't really correspond to any location in the source, so principally they should have empty DebugLocs. Practically, setting these debug locs leads to redundant is_stmts being generated after #108251, causing stepping test failures in the ROCm GDB test suite. Fixes SWDEV-502134
Flow blocks are generated code that don't really correspond to any location in the source, so principally they should have empty DebugLocs. Practically, setting these debug locs leads to redundant is_stmts being generated after llvm#108251, causing stepping test failures in the ROCm GDB test suite. Fixes SWDEV-502134
This patch follows on from the changes made in 5fef40c, by adding an additional heuristic that prevents us from applying the start-of-MBB is_stmt flag when we can see that, for all direct branches to the MBB, the last line stepped on before the branch is the same as the first line of the MBB. This is mainly to prevent certain pathological cases, such as macros that expand to multiple basic blocks that all have the same source location, from giving us repeated steps on the same line. This approach is not comprehensive, since it relies on analyzeBranch to read edges, but the default fallback of applying is_stmt may lead only to useless steps in some cases, rather than skipping useful steps altogether.